do not allow unsupported server URLs as arguments #423
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Caution Review failedThe pull request is closed. WalkthroughConsolidates CLI error handling via print_error, shifts identifier parsing to ID-only (removes URL branch), updates logger tail to app ID/config ID addressing and internal URL resolution, tightens API client log response to logEntries only, and updates help texts. Some signatures changed accordingly. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as CLI tail_logs
participant Utils as parse_app_identifier / resolve_server_url
participant API as MCP App API
participant SSE as Log Stream (SSE)
User->>CLI: tail-logs app_identifier [--stream/--since ...]
CLI->>Utils: parse_app_identifier(identifier)
Note right of Utils: Returns (app_id, config_id)<br/>Raises ValueError on invalid
Utils-->>CLI: app_id, config_id
alt streaming mode
CLI->>Utils: resolve_server_url(app_id/config_id)
Utils-->>CLI: server_url
CLI->>SSE: Connect to server_url with credentials
SSE-->>CLI: log events
CLI->>CLI: filter/format (optional grep/format)
CLI-->>User: stream output
else fetch mode
CLI->>API: GET /logs?app_id/config_id&since&limit&order
API-->>CLI: GetAppLogsResponse (logEntries)
CLI->>CLI: map to list (model_dump), filter/format
CLI-->>User: printed logs
end
opt errors
CLI->>CLI: print_error(...) and/or raise CLIError
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This stack of pull requests is managed by Graphite. Learn more about stacking. |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
|
||
| if not server_url: | ||
| server_url = await resolve_server_url(app_id, config_id, credentials) | ||
| server_url = await resolve_server_url(app_id, config_id, credentials) |
There was a problem hiding this comment.
The removal of the null check for server_url creates a potential issue. The resolve_server_url() function may return None in error cases, which would cause a runtime error when urlparse(server_url) is called on line 256.
Consider either:
- Restoring the null check with appropriate error handling:
server_url = await resolve_server_url(app_id, config_id, credentials)
if not server_url:
console.print("[red]Error: Could not resolve server URL[/red]")
raise typer.Exit(1)- Ensuring
resolve_server_url()never returnsNoneby handling errors internally and raising appropriate exceptions that can be caught by the existing error handlers.
This will prevent unexpected crashes when streaming logs from servers that cannot be resolved.
| server_url = await resolve_server_url(app_id, config_id, credentials) | |
| server_url = await resolve_server_url(app_id, config_id, credentials) | |
| if not server_url: | |
| console.print("[red]Error: Could not resolve server URL[/red]") | |
| raise typer.Exit(1) |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Code LGTM, just small things -- I missed the resolve_server_url implementation earlier but seems like it could be cleaned up a bit later.
I have some reservations about removing the support in general but can appreciate that this is what the task asked for. We do want to use only the server url for configuration though
jtcorbett
force-pushed
the
09-05-do_not_allow_unsupported_server_urls_as_arguments
branch
from
9416bbb to
6456278
Compare
jtcorbett
force-pushed
the
09-05-do_not_allow_unsupported_server_urls_as_arguments
branch
from
6456278 to
edc93a2
Compare
jtcorbett
force-pushed
the
09-05-refactor_logger_tail_http_requests_to_api_client
branch
from
8a35062 to
273b208
Compare
| Raises: | ||
| ValueError: If identifier format is not recognized | ||
| """ | ||
|
|
There was a problem hiding this comment.
The removal of the fallback case return identifier, None, None in favor of raising a ValueError represents a significant breaking change. This change will cause failures for any code that currently passes identifiers not prefixed with app_ or apcnf_.
Consider whether:
- This breaking change is intentional as part of the API redesign
- There are existing callers that rely on the previous behavior
- A more graceful transition might be appropriate, such as:
- Adding a deprecation warning before raising an error
- Providing a migration path for existing code
- Including documentation about the change in release notes
If the strict validation is indeed the desired behavior, ensure that all calling code has been updated to use the new format requirements.
| # Fallback with deprecation warning for backward compatibility | |
| import warnings | |
| warnings.warn( | |
| f"Identifier '{identifier}' does not match expected format with 'app_' or 'apcnf_' prefix. " | |
| "Returning (identifier, None, None) for backward compatibility. " | |
| "This behavior is deprecated and will raise ValueError in a future release. " | |
| "Please update your code to use properly formatted identifiers.", | |
| DeprecationWarning, | |
| stacklevel=2 | |
| ) | |
| return identifier, None, None | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
Merge activity
|
jtcorbett
changed the base branch from
09-05-refactor_logger_tail_http_requests_to_api_client
to
graphite-base/423
jtcorbett
force-pushed
the
09-05-do_not_allow_unsupported_server_urls_as_arguments
branch
from
897130c to
0998161
Compare
jtcorbett
force-pushed
the
graphite-base/423
branch
from
273b208 to
1546ad1
Compare
jtcorbett
changed the base branch from
graphite-base/423
to
09-05-refactor_logger_tail_http_requests_to_api_client
jtcorbett
changed the base branch from
09-05-refactor_logger_tail_http_requests_to_api_client
to
graphite-base/423
jtcorbett
force-pushed
the
09-05-do_not_allow_unsupported_server_urls_as_arguments
branch
from
0998161 to
1dd21da
Compare
TL;DR
Improved app identifier handling by removing URL support and enforcing proper ID formats.
What changed?
parse_app_identifier()to only accept proper app IDs (app_...) or app configuration IDs (apcnf_...), removing URL support(app_id, config_id)instead of including server URLHow to test?
Try using the logger tail command with proper app IDs:
Verify that using URLs now fails with a clear error message:
Test server commands with proper IDs:
Why make this change?
This change standardizes how we identify apps and configurations throughout the CLI, making the interface more consistent and reducing confusion. By enforcing proper ID formats and providing clear error messages, users will better understand what identifiers are expected. Removing URL support simplifies the codebase and creates a more predictable user experience.
Summary by CodeRabbit
Refactor
Documentation
Style